Feature/bca/space explore pagination #3926
Conversation
BillCarsonFr
added
A-Spaces
| * @param limit a client-defined limit to the maximum number of rooms to return per page. Must be a non-negative integer. | ||
| * @param maxDepth: Optional: The maximum depth in the tree (from the root room) to return. | ||
| * @param from: Optional. Pagination token given to retrieve the next set of rooms. Note that if a pagination token is provided, | ||
| * then the parameters given for suggested_only and max_depth must be the same. |
There was a problem hiding this comment.
max_depth is not a part of this API, the SDK forces the value to 1.
So either expose max_depth to the client applications here, or update this comment
| // } | ||
| // } | ||
| // } | ||
| // } |
There was a problem hiding this comment.
Does it worth commenting all the code about auto-join? Maybe just make sure that the value is always set to false?
There was a problem hiding this comment.
auto-join was removed and will probably not be designed like this, I commented out just to remember where it was done to help future implementation
| }.orEmpty() | ||
| } | ||
| .orEmpty() | ||
| SpaceHierarchySummary( |
There was a problem hiding this comment.
SpaceHierarchySummary name is a bit confusing to me (at least at first sight). I proposeSpaceHierarchyData, WDYT?
|
|
||
| internal interface SpaceApi { | ||
|
|
||
| /** | ||
| * @param limit: Optional: a client-defined limit to the maximum number of rooms to return per page. Must be a non-negative integer. | ||
| * @param max_depth: Optional: The maximum depth in the tree (from the root room) to return. |
There was a problem hiding this comment.
maxDepth is the actual parameter name
| * The fields are the same as those returned by /publicRooms (see spec), with the addition of: | ||
| * room_type: the value of the m.type field from the room's m.room.create event, if any. | ||
| * children_state: The m.space.child events of the room. | ||
| * For each event, only the following fields are included1: type, state_key, content, room_id, sender, |
| @@ -62,7 +62,7 @@ class SpaceCardRenderer @Inject constructor( | |||
| inCard.matrixToAccessImage.isVisible = true | |||
| inCard.matrixToAccessImage.setImageResource(R.drawable.ic_room_private) | |||
| } | |||
| val memberCount = spaceSummary.otherMemberIds.size | |||
| val memberCount = spaceSummary.joinedMembersCount?.let { it - 1 } ?: 0 | |||
There was a problem hiding this comment.
Why decrementing the value here? Maybe add a comment to clarify
There was a problem hiding this comment.
The summay object here is constructed from space summary API, so it does not have all the info as a real summary from DB would have (like the list of user ids of members). So I switched to using joinedMembersCount, but i believe that joinedMemberCount is counting current user unlike otherMemberIds, so decremented to keep same result as before.
I'll check if I can't just use that number indeed
| errorWithRetryItem { | ||
| id("error_${currentRootId}_${hierarchySummary.nextToken}") | ||
| text(host.errorFormatter.toHumanReadable(paginationStatus.error)) | ||
| listener { host.listener?.retry() } |
There was a problem hiding this comment.
I think calling retry() is not correct here, you should call host.listener?.loadAdditionalItemsIfNeeded()
There was a problem hiding this comment.
I wanted to be on the safe side and retry all since the beginning, but I can just retry pagination
| session.getRoomSummary(it.childRoomId) | ||
| ?.takeIf { it.membership == Membership.JOIN } // only take if joined because it will be up to date (synced) | ||
| } | ||
| }.distinctBy { it.roomId } |
There was a problem hiding this comment.
Why adding this call to distinctBy()? The server can send duplicates? If it is the case, it's probably better to handle that SDK side.
| errorWithRetryItem { | ||
| id("error_$nextToken") | ||
| text(host.errorFormatter.toHumanReadable(paginationStatus.error)) | ||
| listener { host.listener?.retry() } |
There was a problem hiding this comment.
host.listener?.loadAdditionalItemsIfNeeded() ?
BillCarsonFr
force-pushed
the
feature/bca/space_explore_pagination
branch
from
575f600 to
1c7e567
Compare
Fixes #3693
This PR add supports for the Space Summary API pagination (deprecation of
org.matrix.msc2946/rooms/{roomId}/spacesin favor oforg.matrix.msc2946/rooms/{roomId}/hierarchy)Element Android only uses the hierarchy API with depth 1.
The pagination is triggered seamlesly when the user reaches the last item of the list (using epoxy visibility tracker on a bottom loading item). Pagination results are cached and kept in memory as long as the screen is visible.
This PR is also doing a quick cleaning by removing support for auto-join, that has been postponed for a later release
Here is a sample with some low page size:
